image/internal: sha512 support for skopeo copy #475

lsm5 · 2025-11-17T16:30:21Z

See individual commits.

Vendored by containers/skopeo#2747

podmanbot · 2025-11-17T16:31:36Z

✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6507

packit-as-a-service · 2025-11-17T16:33:30Z

Packit jobs failed. @containers/packit-build please check.

lsm5 · 2025-11-17T16:43:30Z

/packit rebuild-failed

lsm5 · 2025-11-17T20:56:13Z

@mtrmac PTAL

mtrmac

Thanks!

ACK to the image part; the dir: part needs a bit more.

mtrmac · 2025-11-18T21:14:59Z

image/directory/directory_dest.go

 	}()

-	digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo)
+	digester, stream := putblobdigest.DigestIfUnknown(stream, inputInfo)


I think the destination-transport-side digest computation must be a more complex logic, see in the earlier PR about the interaction with cannotModifyManifestReason.

… and dirReference.layerPath discards the algorithm name; that does not generalize for other algorithms, we need to move towards agility where adding an extra algorithm is a ~parameter change and does not require any more changes to the “code proper”; i.e. discarding algorithm names is no longer much of an option.

(We need to keep the existing file names for sha256, to retain compatibility. And… do we define a new value for versionPath?!)

Updated PutBlob to store blob under provided digest algorithm with the algorithm name prepended (except for Canonical) along with a canonical digest hardlink.

$ /usr/bin/ls dc518581817f4e75a7dcfd35383e67c3ef85438250c17e10090b5a31ab8f68d4 manifest.json sha512-2ee373e378345b35e7966a106c5c0a40a005a13bfc87695d89c5bb217f969c351e73810cf78d3d841237098731cf76878f05af8f8d28176c316681f9422ff688 version $ diff dc518581817f4e75a7dcfd35383e67c3ef85438250c17e10090b5a31ab8f68d4 sha512-2ee373e378345b35e7966a106c5c0a40a005a13bfc87695d89c5bb217f969c351e73810cf78d3d841237098731cf76878f05af8f8d28176c316681f9422ff688 $

do we define a new value for versionPath?!)

Doesn't break existing behaviour but there's new stuff, so maybe we should? I'll defer to you.

Hard links are not supported by all file systems (FAT)

Symbolic links are not supported by all file systems either, and generally restricted to admin users on Windows

And, anyway, readers of dir: can only start with the manifest, and the values provided in the manifest. So if PutBlob returns sha512, and the manifest is written to include sha512, readers will not know the sha256 value and have no way to use it.

So I don’t think we need to compute both digests at all; just the layerPath changes to the path computation, + some (as-yet-undefined) logic for PutBlob to use “the algorithm the user wanted”, should be sufficient.

do we define a new value for versionPath?!)

I think with the changes to layerPath, we need to. Previously it was, hypothetically, possible to read a complete sha512 image from dir:, and those images will now break. And we will need to update both dir…Dest… and dir…Src…: destinations should refuse to work on future versions, and still assign the existing 1.1 version for sha256 images for maximum compatibility. sources should detect+refuse future versions.

Consider making the dir changes a separate PR.

I'll keep the dir changes here because of the review comments. Separate PR for image/internal at #486

image/internal/image/digest_validation.go

image/internal/image/docker_schema2.go

image/internal/image/digest_validation.go

lsm5 · 2025-11-19T21:36:08Z

@mtrmac PTA(nother)L . I think I have addressed all comments so far. Used Claude too, but hopefully no AI slop this time 🤞 . LMK.

mtrmac

The internal/image part LGTM.

image/directory/directory_transport.go

mtrmac · 2025-11-22T00:36:55Z

image/directory/directory_dest.go

 	}()

-	digester, stream := putblobdigest.DigestIfCanonicalUnknown(stream, inputInfo)
+	digester, stream := putblobdigest.DigestIfUnknown(stream, inputInfo)


Hard links are not supported by all file systems (FAT)

Symbolic links are not supported by all file systems either, and generally restricted to admin users on Windows

And, anyway, readers of dir: can only start with the manifest, and the values provided in the manifest. So if PutBlob returns sha512, and the manifest is written to include sha512, readers will not know the sha256 value and have no way to use it.

So I don’t think we need to compute both digests at all; just the layerPath changes to the path computation, + some (as-yet-undefined) logic for PutBlob to use “the algorithm the user wanted”, should be sufficient.

do we define a new value for versionPath?!)

I think with the changes to layerPath, we need to. Previously it was, hypothetically, possible to read a complete sha512 image from dir:, and those images will now break. And we will need to update both dir…Dest… and dir…Src…: destinations should refuse to work on future versions, and still assign the existing 1.1 version for sha256 images for maximum compatibility. sources should detect+refuse future versions.

Consider making the dir changes a separate PR.

mtrmac

(Just a brief skim…)

mtrmac · 2025-11-25T20:45:11Z

image/directory/directory_transport.go

+	if d.Algorithm() == digest.Canonical {
+		filename = d.Encoded()
+	} else {
+		filename = d.Algorithm().String() + "-" + d.Encoded()


I wonder whether we need to do a version check and conditionally use the old logic here. I guess that doesn’t matter in practice.

image/directory/directory_src.go

image/directory/directory_dest.go

lsm5 · 2025-11-26T15:59:04Z

good for another look.

mtrmac

A full review now. ACK to the feature set.

mtrmac · 2025-11-27T19:02:16Z

image/directory/directory_dest.go

 var ErrNotContainerImageDir = errors.New("not a containers image directory, don't want to overwrite important data")

+// ErrUnsupportedVersion indicates that the directory uses a version newer than we support
+var ErrUnsupportedVersion = errors.New("unsupported directory transport version")


This works but prevents us from adding more data to the error in the future as an API (e.g. path, the version). type UnsupportedVersionError struct{}, at least? Yes, there’s a pre-existing instance just above…

mtrmac · 2025-11-27T19:05:03Z

image/directory/directory_dest.go

 		return private.UploadedBlob{}, err
 	}

+	if blobDigest.Algorithm() != digest.Canonical {


The placement in the ~middle of “finishing a file write” is a bit unintuitive. Either before the Sync, or maybe just before the Rename (but the succeeded logic makes that placement imprecise).

mtrmac · 2025-11-27T19:05:37Z

image/directory/directory_dest.go

 func (d *dirImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
+	versionToWrite := version1_1
+	if d.usesNonSHA256Digest {
+		versionToWrite = version


This looks confusing — better to name the new one version1_2 explicitly.

mtrmac · 2025-11-27T19:12:13Z

image/directory/directory_test.go

 	assert.Equal(t, signatures, sigs)
 }

+func TestVersionAssignment(t *testing.T) {


Let’s move closer to a typical structure — place this in directory_dest_test.go, please

This can be much shorter as a table-driven test (list of algorithms -> expected version)

mtrmac · 2025-11-27T19:19:08Z

image/directory/directory_dest.go

-				if string(contents) != version {
+				versionStr := string(contents)
+				if versionStr != version && versionStr != version1_1 {
+					if versionStr > version {


I’m not much of a fan of this lexicographic comparison — I think it might be time to start explicitly parsing into ([prefix must match], major, minor), and formatting from integers into strings as well — but at the very least please factor this comparison (“isUnsupportedVersion??”) into a single function shared between sources and destinations.

Maybe share the whole reading + parsing of the file into (major, minor)… that would be more ceremony.

image/directory/directory_dest.go

When storing blobs with non-canonical digest algorithms (e.g., sha512), store the blob under the provided digest algorithm with an algorithm prefix (e.g., "sha512-abc" instead of just "abc"). SHA256 (canonical) digests continue to be stored without a prefix for backward compatibility. Signed-off-by: Lokesh Mandvekar <[email protected]>

Introduce version 1.2 and dynamically assign versions based on the digest algorithms used: - Version 1.1 for sha256-only images (backward compatibility) - Version 1.2 for images using non-sha256 digest algorithms (e.g., sha512) Add validation in both ImageDestination and ImageSource to: - Assume 1.1 if no version file found in dir transport images - Accept both version 1.1 and 1.2 - Refuse unsupported future versions Co-authored-by: Miloslav Trmač <[email protected]> Signed-off-by: Lokesh Mandvekar <[email protected]>

lsm5 · 2025-11-27T21:35:21Z

@mtrmac good for another look. Thanks!

github-actions bot added the image Related to "image" package label Nov 17, 2025

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 17, 2025

dnm: Vendor changes from containers/container-libs#475

418b1af

podmanbot mentioned this pull request Nov 17, 2025

Sync: image/internal: sha512 support for skopeo copy containers/buildah#6507

Draft

lsm5 marked this pull request as ready for review November 17, 2025 20:56

mtrmac reviewed Nov 18, 2025

View reviewed changes

lsm5 marked this pull request as draft November 19, 2025 13:42

lsm5 force-pushed the digest-redux branch from 3826238 to f8744f3 Compare November 19, 2025 21:33

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 19, 2025

dnm: Vendor changes from containers/container-libs#475

c9396ee

lsm5 marked this pull request as ready for review November 19, 2025 21:35

mtrmac reviewed Nov 22, 2025

View reviewed changes

lsm5 force-pushed the digest-redux branch from f8744f3 to 9833c63 Compare November 25, 2025 20:34

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 25, 2025

dnm: Vendor changes from containers/container-libs#475

c948009

lsm5 force-pushed the digest-redux branch from 9833c63 to 983dbfe Compare November 25, 2025 20:37

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 25, 2025

dnm: Vendor changes from containers/container-libs#475

e6e13cb

mtrmac reviewed Nov 25, 2025

View reviewed changes

lsm5 marked this pull request as draft November 26, 2025 15:09

lsm5 force-pushed the digest-redux branch from 983dbfe to d1587a4 Compare November 26, 2025 15:19

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 26, 2025

dnm: Vendor changes from containers/container-libs#475

49a2ee7

lsm5 marked this pull request as ready for review November 26, 2025 15:58

lsm5 mentioned this pull request Nov 26, 2025

image/manifest: Add DigestWithAlgorithm function #499

Open

mtrmac reviewed Nov 27, 2025

View reviewed changes

lsm5 marked this pull request as draft November 27, 2025 19:38

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 27, 2025

dnm: Vendor changes from containers/container-libs#475

41a39bb

lsm5 force-pushed the digest-redux branch from 93c622e to dc04eb8 Compare November 27, 2025 21:09

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 27, 2025

dnm: Vendor changes from containers/container-libs#475

c8d9f22

lsm5 marked this pull request as ready for review November 27, 2025 21:34

image/internal: sha512 support for skopeo copy #475

Are you sure you want to change the base?

image/internal: sha512 support for skopeo copy #475

Conversation

lsm5 commented Nov 17, 2025

Uh oh!

podmanbot commented Nov 17, 2025

Uh oh!

packit-as-a-service bot commented Nov 17, 2025

Uh oh!

lsm5 commented Nov 17, 2025

Uh oh!

lsm5 commented Nov 17, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lsm5 commented Nov 19, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lsm5 commented Nov 26, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsm5 commented Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants